Skip to content

[Custom Transactions] Add TxBuilder trait, support fixed additional outputs #3775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tankyleo
Copy link
Contributor

@tankyleo tankyleo commented May 13, 2025

hi @TheBlueMatt on this branch here i make an attempt at an API that supports "fixed additional outputs" on commitment transactions.

I place today's 330 sat anchors in that bucket, but the API should allow for custom ones.

The API should also let people customize all the existing outputs on the commitment transaction.

Still need to think about any remaining TODOs for additional outputs that are only present for some commitments (eg. DLC outputs), depending on how we'd like to prioritize these.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented May 13, 2025

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from c245ca1 to 7fc2092 Compare May 13, 2025 04:58
@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from 7fc2092 to 6aa0848 Compare May 21, 2025 04:43
@carlaKC carlaKC mentioned this pull request May 21, 2025
38 tasks
@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from fa3ea56 to 14b83a3 Compare May 22, 2025 05:20
Copy link

codecov bot commented May 22, 2025

Codecov Report

Attention: Patch coverage is 95.10309% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.37%. Comparing base (5316257) to head (14b83a3).
Report is 49 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channel.rs 92.40% 5 Missing and 14 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3775      +/-   ##
==========================================
+ Coverage   89.53%   90.37%   +0.84%     
==========================================
  Files         157      160       +3     
  Lines      125310   133578    +8268     
  Branches   125310   133578    +8268     
==========================================
+ Hits       112192   120721    +8529     
+ Misses      10429    10231     -198     
+ Partials     2689     2626      -63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tankyleo tankyleo requested a review from TheBlueMatt May 22, 2025 05:30
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@carlaKC carlaKC self-requested a review May 29, 2025 17:12
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a reasonable API on first pass! Will have something smarter to say once I've rebased on this 👌

@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 6th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 7th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 8th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 9th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 10th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 11th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 12th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I assume this is kinda blocked until we land more of 0FC?

@@ -622,6 +622,21 @@ impl HTLCOutputInCommitment {
&& self.cltv_expiry == other.cltv_expiry
&& self.payment_hash == other.payment_hash
}

pub(crate) fn is_dust(&self, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want the interface to decide this, shouldn't it just be a new flag in HTLCOutputInCommitment rather than a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appended a commit below that makes this a flag - hopefully I've understood you :) Assuming for now we don't let users of TxBuilder decide which HTLCs are dust-vs-nondust.

let mut value_to_self_msat_offset = 0;

let feerate_per_kw = feerate_per_kw.unwrap_or_else(|| self.get_commitment_feerate(funding, generated_by_local));

for htlc in self.pending_inbound_htlcs.iter() {
if htlc.state.included_in_commitment(generated_by_local) {
if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume in the future this will be something the TxBuilder decides?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.

I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.

Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.

I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.

Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.

Copy link
Contributor Author

@tankyleo tankyleo Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.

Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."

I do see your point about removing from channel all branches on channel type. IIUC this is not a blocker for this PR let me know.

Copy link
Contributor Author

@tankyleo tankyleo Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.

Sounds good I'll push a refactor as a follow-up to this PR is that ok ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."

Hmm, yea, would be nice to just have a "commitment transactions are not the responsibility of channel.rs" goal. Doesn't have to get there now, but eventually would be nice to not have channel.rs have any concept for commitment transactions, only stats on them.

@tankyleo
Copy link
Contributor Author

tankyleo commented Jun 20, 2025

Makes sense. I assume this is kinda blocked until we land more of 0FC?

Yes would love to merge this as part of the 0FC PR sequence ! Also would like to merge #3855 first, to make sure whatever API we settle on here fits in nicely with a splicing world; in that world for a single set of included HTLCs based on their state, we generate multiple commitments, so this is relevant to this PR here.

@TheBlueMatt
Copy link
Collaborator

Yes would love to merge this as part of the 0FC PR sequence !

Oh! Okay, I thought we were gonna do 0FC first, but happy to do this first.

@TheBlueMatt
Copy link
Collaborator

What's left to get this out of draft, then? Want to make quick progress on 0FC.

@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from 14b83a3 to 2541f45 Compare June 23, 2025 17:44
@tankyleo tankyleo marked this pull request as ready for review June 23, 2025 17:44
@tankyleo tankyleo removed the request for review from TheBlueMatt June 23, 2025 18:18
@carlaKC
Copy link
Contributor

carlaKC commented Jun 23, 2025

I think that we can make progress on TxBuilder and the off-chain logic in #3884 in parallel, and then the on-chain changes for 0FC will depend on TxBuilder. Wrote up ordering in #3789.

@tankyleo tankyleo force-pushed the fixed-addl-outputs branch 3 times, most recently from 9dba3c2 to 2f19710 Compare June 24, 2025 04:40
@tankyleo tankyleo requested review from TheBlueMatt and carlaKC June 24, 2025 06:32
@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from 0071eff to 5d6baeb Compare June 24, 2025 06:44
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments, do you want to pick a second reviewer or should the bot do it?

if match update_state {
// Note that these match the inclusion criteria when scanning
// pending_inbound_htlcs below.
FeeUpdateState::RemoteAnnounced => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that its super critical in this case, but when moving code, please move in a separate commit from rustfmt, such that git show --color-moved --color-moved-ws=ignore-space-change can highlight the move.

remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits
#[derive(Debug, PartialEq)]
pub(crate) struct CommitmentStats {
pub(crate) total_fee_sat: u64, // the total fee included in the transaction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanna go ahead and make the comments documentation?

#[derive(Debug, PartialEq)]
pub(crate) struct CommitmentStats {
pub(crate) total_fee_sat: u64, // the total fee included in the transaction
pub(crate) local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are now wrong, as the fields are now post-anchors.

@@ -4146,31 +4172,21 @@ where
{
let remote_commit_tx_fee_msat = if funding.is_outbound() { 0 } else {
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
self.next_remote_commit_tx_fee_msat(funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations
self.next_remote_commit_tx_fee_msat(&funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like unnecessary diff to add the & before funding here and below.

// `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
// the spec because the fee spike buffer requirement doesn't exist on the receiver's
// side, only on the sender's. Note that with anchor outputs we are no longer as
// sensitive to fee spikes, so we need to account for them.
//
// A `None` `HTLCCandidate` is used as in this case because we're already accounting for
// the incoming HTLC as it has been fully committed by both sides.
let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(funding, None, Some(()));
let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(&funding, None, Some(()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include the extra dust-buffer HTLC in the build_commitment_stats call, above, to match?

Copy link
Contributor Author

@tankyleo tankyleo Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion. I looked into this, here's the major difference between the two paths:

  • next_remote_commit_tx_fee_msat will set the dust threshold for HTLCs based on context.feerate_per_kw
  • the number of HTLCs passed to build_commitment_stats above come from get_pending_htlc_stats, which itself will set the HTLC dust threshold according to context.get_dust_buffer_feerate(outbound_feerate_update)

Therefore number of nondust HTLCs will differ.
Therefore, commit_tx_fee_sat will differ.

cltv_expiry: cltv_expiry.0.unwrap(),
payment_hash: payment_hash.0.unwrap(),
transaction_output_index,
is_dust: transaction_output_index.is_none(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, should this just be an fn that checks if the output index is none? Would imply passing something other than HTLCOutputInCommitment to build_commitment_transaction (as at that point it doesn't have an output index), but IMO that's a cleaner API anyway.

@tankyleo
Copy link
Contributor Author

Few comments, do you want to pick a second reviewer or should the bot do it?

Thanks I pinged Carla how does that sound ?

@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from 5d6baeb to 3f98b95 Compare June 25, 2025 00:16
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started review yesterday so please ignore any outdated comments!

Comment on lines 1134 to 1135
local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits
remote_balance_before_fee_msat: u64, // remote balance before fees and anchors *not* considering dust limits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment remove "and anchors" from comments

value_to_self_after_htlcs: u64, value_to_remote_after_htlcs: u64,
channel_type: &ChannelTypeFeatures,
) -> CommitmentStats {
let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, nondust_htlc_count, channel_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a goal be to allow implementations of TxBuilder to pick their own feerate (ie, not the value that commit_tx_fee_sat would set?

If yes, transaction_fee_satoshis would be wrong in get_available_balances. Doesn't look like this is actually used, but it would surface an incorrect value to end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you ! In that call, I am wondering whether we can subtract the value of the outputs from some channel_value_satoshis value and get the total fee that way... will think more this afternoon !

total_fee_sat,
local_balance_before_fee_msat,
remote_balance_before_fee_msat: _,
} = SpecTxBuilder {}.build_commitment_stats(true, commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, value_to_self_msat, push_msat, &channel_type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: here and a few others, move parameters onto newlines to save us needing to rustfmt them later

if holder_balance_msat < buffer_fee_msat + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 {
let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize));
let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat;
// Note that `stats.total_fee_sat` now accounts for any HTLCs that transition from non-dust to dust under a higher feerate (in the case where HTLC-transactions pay endogenous fees).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc previously we were using the old commitment feerate to build our commitment transaction (and thus possibly not trimming some HTLCs that became dust), and now we're using the new proposed feerate so we'll properly trim them to dust and account for them in our fees?

Would be nice to have a test before this commit that demonstrates the htcs not being properly trimmed, and then updated in this commit to show that they are.

funding.get_channel_type(),
);

// Note that this is now the capacity available after we have subtracted any non-zero-value anchors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(very) nitpicky: remove "this is now" from comment, here and above. Somebody reading this code from fresh won't be looking at the diff, so the comparison to the past state doesn't help them (commit message seems like a a better home for notes on what's changed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly I tend to annotate my code for reviewers using comments like these :) Will move this to the git message thank you

Comment on lines 5183 to 5185
// We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment
// transaction won't be generated until they send us their next RAA, which will mean
// dropping any HTLCs in this state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Move comment up to above the if-let? Bit out of place here

let extra_htlc_commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat;

let htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type());
let commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder whether we've got the right API here when we're making calls like this passing zero values for local/remote amounts (and passing a feerate that we don't need in another place).

Did you consider splitting some of the components of CommitmentStats out into their own methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I did, I agree with you I am not thrilled about this - we had a back and forth with Matt on this issue, will refresh on this this afternoon :)

@@ -5102,9 +5132,15 @@ where
fn next_local_commit_tx_fee_msat(
&self, funding: &FundingScope, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>,
) -> u64 {
let context = &self;
let context = self;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary & added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see seems this removes the & ?

Comment on lines 5206 to 5207
funding.value_to_self_msat.saturating_sub(local_htlc_total_msat),
(funding.get_value_satoshis() * 1000).checked_sub(funding.value_to_self_msat).unwrap().saturating_sub(remote_htlc_total_msat),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull these out into their own vars here and in next_local_commit_tx_fee_msat to help readability a bit?

let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
let mut htlcs_included: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs);
let mut nondust_htlcs: Vec<HTLCOutputInCommitment> = Vec::with_capacity(num_htlcs);
let mut value_to_self_msat_offset = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking/personal preference: WDYT about tracking value_to_self_claimed and value_to_remote_claimed as separate u64 values and then used checked_sub below? For the sake of avoiding negative numbers / slight readability improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great thank you a prior goal of mine was to remove all the raw as casts here this one was the last one remaining !

@tankyleo tankyleo self-assigned this Jun 26, 2025
tankyleo added 13 commits June 27, 2025 00:27
…Builder`

This is a temporary type that will be removed once we let `TxBuilder` choose
which HTLCs are dust and which are non-dust.
In addition to the amount set by `commit_tx_fee_sat`, the transaction
fee of a commitment transaction may also increase due to HTLC amounts
that get burned to fees, and any elided anchors. We now include these
amounts in the `transaction_fee_satoshis` field of
`Balance::ClaimableOnChannelClose` too.

This commit also makes the `transaction_fee_satoshis` field correct for
custom transactions not encompassed in `chan_utils::commit_tx_fee_sat`.
@tankyleo tankyleo force-pushed the fixed-addl-outputs branch from 3f98b95 to a9892a9 Compare June 27, 2025 06:13
Copy link
Contributor Author

@tankyleo tankyleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major changes:

  • Split out build_commitment_stats, see 660b86e
  • Introduce IncludedHTLC type to enforce the dust status of a HTLC on TxBuilder without specifying the particular index, see f736d90
    • We will let TxBuilder set this status in the next version of this API, for now we focus on its use in zero-fee commitments.
  • Balance::ClaimableOnChannelClose.transaction_fee_satoshis now includes the sum of the fee due to dust HTLCs, and non-dust HTLCs getting rounded down, see a9892a9
  • Introduce standalone commit to remove the last raw cast in build_commitment_stats, see e26b232

Remaining TODOs:

  • Append a commit to remove all rustfmt::skip in the code we touched.
  • Write a test to demonstrate how we were previously not accounting for the decrease in weight due to newly trimmed HTLCs when checking whether we can afford a new feerate in can_send_update_fee
  • Proper git messages

Full diff

Comment on lines +4802 to +4803
let extra_htlc_commit_tx_fee_sat = SpecTxBuilder {}.commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type());
let extra_htlc_htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we expand the commit_tx_fee_sat call to also pass information back about the fees on HTLC transactions ?

@tankyleo tankyleo requested a review from carlaKC June 27, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants